Split rcl_bindings.cpp file #1100
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR splits the large rcl_bindings.cpp file into several smaller, dedicated binding files to improve modularity and maintainability. Key changes include the addition of new files (e.g. rcl_node_bindings.cpp, rcl_names_bindings.cpp, rcl_logging_bindings.cpp, etc.), updates to the addon.cpp and binding.gyp files to reference these new modules, and renaming of certain initialization functions for consistency.
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rcl_node_bindings.cpp | New binding functions for node operations; extracts parameter wrapping. |
| src/rcl_names_bindings.{h,cpp} | New bindings to support name validations and expansions. |
| src/rcl_logging_bindings.{h,cpp} | New logging bindings for setting logger level and log messages. |
| src/rcl_lifecycle_bindings.{h,cpp} | Renamed lifecycle initializer to InitLifecycleBindings for consistency. |
| src/rcl_context_bindings.cpp | New context bindings with argv allocation; potential allocation issue. |
| addon.cpp | Updated to initialize new binding modules. |
| binding.gyp | Updated file list to include the new binding files. |
src/rcl_context_bindings.cpp
Outdated
| for (int i = 0; i < argc; i++) { | ||
| std::string arg = jsArgv.Get(i).As<Napi::String>().Utf8Value(); | ||
| int len = arg.length() + 1; | ||
| argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char*))); |
There was a problem hiding this comment.
The allocation for argv[i] uses sizeof(char*) instead of sizeof(char), which may allocate an incorrect amount of memory. Replace sizeof(char*) with sizeof(char) for proper allocation.
| argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char*))); | |
| argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char))); |
There was a problem hiding this comment.
Pull Request Overview
This PR splits the original monolithic rcl_bindings.cpp file into multiple dedicated binding files for different ROS components (node, names, logging, lifecycle, graph, context, client, etc.) and updates the build configuration accordingly.
- Separation of binding code into multiple files for improved modularity and maintainability
- Renaming and updating of several initialization functions (e.g. InitLifecycleBindings, InitActionBindings)
- Updating the binding.gyp to include the new source files
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rcl_node_bindings.cpp | Added bindings for node-related functionality |
| src/rcl_names_bindings.h/.cpp | Introduced new bindings for name-related utilities |
| src/rcl_logging_bindings.h/.cpp | Added logging bindings with functions to set and get logger levels |
| src/rcl_lifecycle_bindings.{h,cpp} | Renamed and updated lifecycle bindings to InitLifecycleBindings |
| src/rcl_guard_condition_bindings.{h,cpp} | New guard condition bindings |
| src/rcl_graph_bindings.{h,cpp} | New graph bindings for publisher/subscriber/service information |
| src/rcl_context_bindings.{h,cpp} | Context initialization and shutdown bindings |
| src/rcl_client_bindings.{h,cpp} | New bindings to support client creation and request management |
| src/rcl_bindings.h | Removed deprecated declarations now managed in separate files |
| src/rcl_action_bindings.{h,cpp} | Renamed action binding functions to InitActionBindings |
| src/addon.cpp | Updated module initialization to load the new binding modules |
| binding.gyp | Added new source files for all the separated binding modules |
| rcl_get_subscriber_names_and_types_by_node( | ||
| node, &allocator, no_demangle, node_name.c_str(), | ||
| node_namespace.c_str(), &topic_names_and_types), | ||
| "Failed to get_publisher_names_and_types."); |
There was a problem hiding this comment.
In GetSubscriptionNamesAndTypesByNode, the error message incorrectly refers to 'publisher' instead of 'subscriber'. Consider updating the error string to 'Failed to get_subscriber_names_and_types.'
| "Failed to get_publisher_names_and_types."); | |
| "Failed to get_subscriber_names_and_types."); |
| rcl_get_subscriber_names_and_types_by_node( | ||
| node, &allocator, no_demangle, node_name.c_str(), | ||
| node_namespace.c_str(), &topic_names_and_types), | ||
| "Failed to get_publisher_names_and_types."); |
There was a problem hiding this comment.
In GetServiceNamesAndTypesByNode, the error message erroneously references 'publisher' instead of 'service'. It should likely read 'Failed to get_service_names_and_types.'
| "Failed to get_publisher_names_and_types."); | |
| "Failed to get_subscriber_names_and_types."); |
| RCL_RET_OK, | ||
| rcl_get_topic_names_and_types(node, &allocator, no_demangle, | ||
| &topic_names_and_types), | ||
| "Failed to get_publisher_names_and_types."); |
There was a problem hiding this comment.
In GetTopicNamesAndTypes, the error message incorrectly mentions 'publisher' instead of 'topic'. Updating the message to 'Failed to get_topic_names_and_types.' would improve clarity.
| "Failed to get_publisher_names_and_types."); | |
| "Failed to get_topic_names_and_types."); |
This PR splits the large rcl_bindings.cpp file into several smaller, dedicated binding files to improve modularity and maintainability. Key changes include the addition of new files (e.g. rcl_node_bindings.cpp, rcl_names_bindings.cpp, rcl_logging_bindings.cpp, etc.), updates to the addon.cpp and `binding.gyp` files to reference these new modules, and renaming of certain initialization functions for consistency. Meanwhile, this patch fixes the GitHub actions failure for Linux platform. Fix: #1099
1 participant
This PR splits the large rcl_bindings.cpp file into several smaller, dedicated binding files to improve modularity and maintainability. Key changes include the addition of new files (e.g. rcl_node_bindings.cpp, rcl_names_bindings.cpp, rcl_logging_bindings.cpp, etc.), updates to the addon.cpp and
binding.gypfiles to reference these new modules, and renaming of certain initialization functions for consistency.Meanwhile, this patch fixes the GitHub actions failure for Linux platform.
Fix: #1099